fix(stream) handle task switching #3609
Conversation
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
|
@cursor review |
|
You have used all Bugbot PR reviews included in your free trial for your GitHub account on this workspace. To continue using Bugbot reviews, enable Bugbot for your team in the Cursor dashboard. |
Greptile SummaryThis PR fixes task-switching behaviour in the streaming chat system: when a user navigates back to a task that has an ongoing stream, the hook now detects the presence of an Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant useChat
participant QueryClient
participant Server
Note over User,Server: Task switching — happy path (snapshot available)
User->>useChat: Switch to Task B (initialChatId changes)
useChat->>useChat: Reset state, appliedChatIdRef = undefined
useChat->>QueryClient: useChatHistory(taskBId)
QueryClient->>Server: GET /api/copilot/chat?chatId=taskB
Server-->>QueryClient: { activeStreamId, streamSnapshot: {...} }
QueryClient-->>useChat: chatHistory updated
useChat->>useChat: snapshot present → skip early return
useChat->>useChat: appliedChatIdRef = chatHistory.id
useChat->>useChat: setMessages, reconnect SSE
Note over User,Server: Task switching — no snapshot yet (polling loop risk)
User->>useChat: Switch to Task C (stream just started)
useChat->>QueryClient: useChatHistory(taskCId)
QueryClient->>Server: GET /api/copilot/chat?chatId=taskC
Server-->>QueryClient: { activeStreamId, streamSnapshot: null }
QueryClient-->>useChat: chatHistory updated
useChat->>useChat: activeStreamId && !snapshot → invalidateQueries (NO appliedChatIdRef set)
useChat->>QueryClient: invalidateQueries(taskKeys.detail(taskCId))
QueryClient->>Server: GET /api/copilot/chat?chatId=taskC (refetch)
Server-->>QueryClient: { activeStreamId, streamSnapshot: null } (still no snapshot)
Note over useChat,Server: Loop repeats until snapshot appears or stream ends
Note over User,Server: Deploy tool result — client-side registry update
useChat->>useChat: tool_result for deploy_api / deploy_chat / redeploy
useChat->>useChat: Check output.isDeployed is boolean
useChat->>useChat: setDeploymentStatus in workflowRegistry
useChat->>QueryClient: invalidate deploymentKeys.info + versions + workflowKeys.list
Last reviewed commit: cc1ae03 |
| useEffect(() => { | ||
| return () => { | ||
| streamGenRef.current++ | ||
| // Only drop the browser→Sim read; the Sim→Go stream stays open | ||
| // so the backend can finish persisting. Explicit abort is only | ||
| // triggered by the stop button via /api/copilot/chat/abort. | ||
| abortControllerRef.current?.abort() | ||
| abortControllerRef.current = null | ||
| sendingRef.current = false | ||
| } | ||
| }, []) |
There was a problem hiding this comment.
Removed abort means stream continues processing after unmount
The previous cleanup explicitly called abortControllerRef.current?.abort() to cancel the in-flight SSE fetch when the component unmounts. That line was intentionally removed here to allow the backend stream to stay alive across task switches, but it has a side effect: the ReadableStreamDefaultReader inside processSSEStream's while (true) { reader.read() } loop keeps running after unmount.
streamGenRef.current++ prevents finalizeRef.current() from being called with a stale generation, but the inner flush() calls — which call setMessages, setIsSending, setIsReconnecting etc. — are not gated by the generation counter. If the stream is still in flight when the user switches tasks, state setters are called on the now-stale React fiber, which can cause React warnings and ghost state.
Consider aborting the reader without aborting the server-side stream — i.e., abort the client fetch connection while keeping the Go-side stream alive. The server-side stream can remain open without the client holding a reader:
| } | ||
| await db.delete(chat).where(eq(chat.id, existing[0].id)) | ||
| return { success: true, output: { success: true, action: 'undeploy', isDeployed: false } } | ||
| return { | ||
| success: true, | ||
| output: { workflowId, success: true, action: 'undeploy', isChatDeployed: false }, | ||
| } |
There was a problem hiding this comment.
deploy_chat undeploy response is missing isDeployed for symmetry
The deploy path returns isDeployed: true, but the undeploy path now returns isChatDeployed: false with no isDeployed field. The client-side handlers in both handlers.ts and use-chat.ts gate their registry update on typeof output?.isDeployed === 'boolean', so they silently skip the undeploy case.
This is arguably correct behaviour (undeploying a chat does not remove the workflow's API deployment), but the asymmetry between the deploy and undeploy output shapes makes intent harder to follow. A comment explaining why isDeployed is intentionally absent from the undeploy response would help future maintainers:
| } | |
| await db.delete(chat).where(eq(chat.id, existing[0].id)) | |
| return { success: true, output: { success: true, action: 'undeploy', isDeployed: false } } | |
| return { | |
| success: true, | |
| output: { workflowId, success: true, action: 'undeploy', isChatDeployed: false }, | |
| } | |
| return { | |
| success: true, | |
| // isDeployed is intentionally omitted: undeploying a chat does not | |
| // affect the workflow's API deployment status. | |
| output: { workflowId, success: true, action: 'undeploy', isChatDeployed: false }, | |
| } |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| if (activeStreamId && !snapshot && !sendingRef.current) { | ||
| queryClient.invalidateQueries({ queryKey: taskKeys.detail(chatHistory.id) }) | ||
| return | ||
| } |
There was a problem hiding this comment.
Unbounded polling loop when no snapshot is available
When switching to a task that has an active stream but no snapshot, this early return path intentionally skips setting appliedChatIdRef.current = chatHistory.id. That means every time the invalidated query resolves and chatHistory updates, this useEffect fires again, evaluates the same condition, and calls invalidateQueries once more — creating a tight polling loop for the lifetime of the stream.
useChatHistory has a staleTime of 30 seconds, but invalidateQueries bypasses staleTime and triggers an immediate refetch. So the loop rate is limited only by network round-trip time, which could produce dozens of requests per minute if the server takes a while to produce a snapshot.
If the intent is to poll until a snapshot appears, this needs an explicit back-off or maximum retry count. One approach is to return a cleanup function from the effect that uses a delayed invalidateQueries call so subsequent polls are spaced out, rather than firing immediately on every data update.
* Fix deploy subagent * fix(stream) handle task switching (#3609) * Fix task switching causing stream to abort * Process all task streams all the time * Process task streams that are in the background --------- Co-authored-by: Theodore Li <theo@sim.ai> * Always return isDeployed for undeploy chat * Fix lint --------- Co-authored-by: Siddharth Ganesan <siddharthganesan@gmail.com> Co-authored-by: Theodore Li <theo@sim.ai>
* Fix deploy subagent * fix(stream) handle task switching (#3609) * Fix task switching causing stream to abort * Process all task streams all the time * Process task streams that are in the background --------- Co-authored-by: Theodore Li <theo@sim.ai> * Always return isDeployed for undeploy chat * Fix lint --------- Co-authored-by: Siddharth Ganesan <siddharthganesan@gmail.com> Co-authored-by: Theodore Li <theo@sim.ai>
Summary
Had issues with task streaming.
WIP
Switched to processing all task sse streams even in the background and added handling for deployment tool calls.
Type of Change
Testing
Validated that switching between tasks doesn't cause issues and deployment tools successfully update the workflow status in the workflow view.
Checklist
Screenshots/Videos